Claude finished @jcfs's task in 6m 43s —— View job

Code Review

Todo:

• Gather context and understand the PR
• Review streams.go (lifecycle StreamManager drain barrier)
• Review client.go + workspace_stream.go (agentctl Client.Close drain)
• Review agent.go and manager_interaction.go
• Review client_close_test.go (regression test)
• Review Makefile changes
• Post findings

Overall Assessment

Solid, well-reasoned fix for the CI goleak flake. The two-pronged approach — closing waitCh in sm.Wait() so the SM goroutines drain without depending on external stopCh, and adding streamWG + closed guard to Client.Close() so it becomes a true drain barrier — directly addresses the two independent race windows described in the PR. The stopChannelContext rewrite is careful and correct. Most findings below are documentation/minor correctness notes; there are no blocking issues.

Findings

client.go — streamWG comment misrepresents workspace goroutine tracking
Inline comment — streamWG only tracks readUpdatesStream; workspace goroutines are on stream.wg and drained via explicit ws.Wait(), not "transitively" through streamWG.

client.go — double-close path in Client.Close()
Inline comment — CloseWorkspaceStream() closes the raw *websocket.Conn outside closeOnce, then ws.Close() tries to close it again (logged at Debug). Harmless in practice, but the comment "idempotent via closeOnce" is slightly misleading for this call site. Worth either removing CloseWorkspaceStream() from Close() or adding a note explaining both calls serve different purposes (read-loop exit vs. write-loop wakeup).

streams.go — mergeStops goroutine always spawned
Inline comment — Since secondary (= waitCh) is never nil, Done() always spawns mergeStops, including for connectUpdatesStream's context, which returns immediately after the dial. The goroutine is bounded and correct (exits on Wait()), but the primary == nil && secondary == nil early-return in Done() is now dead code. A short note would help future readers not flag this as a leak.

workspace_stream.go — post-dial section doesn't guard concurrent StreamWorkspace callers
Inline comment — Pre-existing gap: the post-dial second-lock check guards only closed, not workspaceStreamConn != nil. Two goroutines that both pass the pre-dial check could both dial and the second would overwrite workspaceStream, orphaning the first conn's goroutines. StreamManager never calls StreamWorkspace concurrently in practice, so this isn't a regression, but adding the check here would seal it for free.

client_close_test.go — mock.connected semantics
Inline comment — m.once fires on the first connection, not both. Both client goroutines are already live before this wait (both dials returned), so the drain works correctly. A clarifying comment would prevent future readers from reading the once as "both connections are live on the server side."

Minor observations (not inline)

• manager_interaction.go: the switch from execution.agentctl.Close() to CloseUpdatesStream() + CloseWorkspaceStream() is correct and the comment explains the reasoning well.
• Makefile: test-lifecycle-goleak target is a useful addition. The CGO_PREFIX variable on the go test line is consistent with the rest of the file.
• The streamContext always returns a *stopChannelContext now (even for test callers that previously got a plain context). This is intentional — secondary (waitCh) provides the drain signal — but note that every dial now spawns a mergeStops goroutine until sm.Wait().

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Confidence Score: 5/5

Safe to merge — the drain barriers are correctly implemented and the identity guards prevent state corruption during the restart path.

The changes are carefully scoped: waitCh is protected by sync.Once, merge goroutines are correctly registered on sm.wg before they are spawned (inside once.Do while the outer goroutine still holds its own wg count), and the two-phase guard in StreamWorkspace (pre-dial + post-dial closed check) closes the race window without introducing any new lock ordering issues. Identity guards in readWorkspaceStream's defer correctly prevent the old stream from overwriting new-stream pointers during restart. The test directly exercises the drain guarantee and the post-close error path.

No files require special attention — all changes are self-consistent and the implementation matches the documented invariants.

Important Files Changed

_{Reviews (3): Last reviewed commit: "fix(lifecycle): drop agent-stream drain ..." | Re-trigger Greptile}

Claude finished @jcfs's task in 3m 51s —— View job

Claude finished reviewing — follow-up commit 7562635

Todo

• Load tools
• Read updated files for review
• Verify previous review issues are resolved
• Check for new issues in the updated code
• Post inline comments and final assessment

Follow-up Commit Review

The second commit (fix(lifecycle): track mergeStops goroutine and guard workspace stream races) cleanly addresses all issues raised in the first round of review. Here's the resolution status:

One minor residual note

The if c.primary == nil && c.secondary == nil early return in stopChannelContext.Done() (streams.go:74) is dead code now that secondary (sm.waitCh) is always non-nil. This was flagged in round 1 as worth a short note so future readers don't wonder why mergeStops "always" spawns. The logic is correct and bounded — just worth a // secondary (sm.waitCh) is always non-nil so this path is unreachable comment or a cleanup in a follow-up.

wg.Add ordering is correct

The wg.Add(1) inside once.Do runs before go c.mergeStops(), and the outer goroutine cannot call sm.wg.Done() before connectUpdatesStream returns — which is after DialContext (the site that calls ctx.Done() and triggers once.Do). So the WaitGroup counter can never reach 0 before mergeStops is registered. The fix is sound.

Overall: All substantive issues from round 1 are resolved correctly. The fix is minimal, well-documented, and safe to merge.

Claude finished @jcfs's task in 5m 9s —— View job

Code Review

Todo:

• Load tools
• Read changed files in current state (all 3 commits)
• Review streams.go (lifecycle StreamManager drain barrier)
• Review client.go + workspace_stream.go + agent.go (agentctl drain)
• Review manager_interaction.go and Makefile
• Review client_close_test.go
• Check for issues raised in prior reviews
• Post findings

Overall Assessment

Three-commit arc tells a coherent story. Commit 1 added drain barriers for both streams + workspace stream race guards. Commit 2 addressed all round-1 review findings (mergeStops sm.wg tracking, workspaceStreamConn/workspaceStream identity guards, comment accuracy, concurrent StreamWorkspace post-dial check). Commit 3 correctly identified a cascade deadlock introduced by the agent-stream synchronous drain and scoped the fix down to workspace-only — well-documented and sound.

The workspace drain barrier — the original goleak source — is solid. All round-1 and round-2 review issues are resolved. Two residual items noted below.

Findings

1. readUpdatesStream defer lacks identity guard on agentStreamConn — inline

agent.go has no net diff in this PR (commit 1 added changes; commit 3 reverted them), so the comment lands on client.go:695 instead. readWorkspaceStream's defer was fixed in commit 2 to guard both pointer resets with if c.workspaceStreamConn == conn. The analogous guard is absent in readUpdatesStream's defer, which unconditionally writes c.agentStreamConn = nil. Since commit 3 removes the synchronous agent-stream drain, the race window for the restart path is now wider: the old goroutine can outlive both CloseUpdatesStream() and the StreamUpdates dial in initializeACPSessionForRestart, clobbering the new conn pointer. One-line fix: if c.agentStreamConn == conn { ... }. Greptile flagged this as P1; it was acknowledged but didn't make it into the follow-up commit. Fix this →

2. Dead code guard in stopChannelContext.Done() — inline

streams.go:74 — the c.primary == nil && c.secondary == nil early-return is unreachable because streamContext() always wires sm.waitCh (non-nil) as secondary. Worth a comment noting this is defensive or removing the dead branch. Minor.

What's clean

• StreamWorkspace post-dial closed + workspaceStreamConn != nil double-check (commit 2) correctly seals both the shutdown race and the concurrent-caller race.
• stopChannelContext.wg threading into sm.wg ensures mergeStops is tracked even when connectUpdatesStream's outer goroutine exits first — that was the subtler of the two goleak paths.
• RestartAgentProcess comment explaining the per-stream Close vs. full client.Close() trade-off is clear and correct.
• Test updated in commit 3 to match the narrowed contract (workspace drain only) — no dead assertions.
• test-lifecycle-goleak Makefile target is a useful stress hook.